feat(explorer): show network costs and protocol fee breakdown on order details#7588
Open
jmg-duarte wants to merge 15 commits into
Open
feat(explorer): show network costs and protocol fee breakdown on order details#7588jmg-duarte wants to merge 15 commits into
jmg-duarte wants to merge 15 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduce protocol-fee types and extraction (getProtocolFees), add unit tests, render per-fee protocol-fee rows in GasFeeDisplay (with token resolution), add NumbersBreakdown UI, enrich orders in OrderDetails, adjust a ShowMoreButton usage, and bump many ChangesProtocol fee extraction & display
Sequence DiagramsequenceDiagram
participant OrderDetails
participant getProtocolFees
participant GasFeeDisplay
participant useMultipleErc20
OrderDetails->>getProtocolFees: enrichOrderFromTrades(trades)
getProtocolFees->>OrderDetails: protocolFees[]
OrderDetails->>GasFeeDisplay: render(order with protocolFees)
GasFeeDisplay->>useMultipleErc20: request token metadata for fee tokenAddresses
useMultipleErc20->>GasFeeDisplay: token metadata map
GasFeeDisplay->>GasFeeDisplay: resolveToken & formatFee -> render rows
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying explorer-dev with
|
| Latest commit: |
f051cfc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e6f10d01.explorer-dev-dxz.pages.dev |
| Branch Preview URL: | https://jmgd-ucp.explorer-dev-dxz.pages.dev |
Deploying swap-dev with
|
| Latest commit: |
f051cfc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e778039a.swap-dev-5u6.pages.dev |
| Branch Preview URL: | https://jmgd-ucp.swap-dev-5u6.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/explorer/src/test/utils/operator/orderFees.test.ts (1)
81-89: ⚡ Quick winAdd a mixed-checksum aggregation regression test.
Current coverage checks case-insensitive comparison, but not aggregation across multiple trades where the same token appears in different casing. Add one test with two trades (
FEE_TOKENandFEE_TOKEN.toLowerCase()) and assert single-token aggregation without warning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/explorer/src/test/utils/operator/orderFees.test.ts` around lines 81 - 89, Add a regression test in orderFees.test.ts that verifies aggregation across mixed-checksum token strings: create two trades using makeTrade — one with token FEE_TOKEN and one with FEE_TOKEN.toLowerCase() — pass them to getFees alongside an order from makeOrder with executedFeeToken set to a different-cased FEE_TOKEN and totalFee large enough to split protocol/network fees, then assert the returned protocolFees equals the sum of both trade amounts (as a single token aggregation) and networkCosts equals the expected remainder; also assert no warning was emitted (e.g., result.warnings is empty or console.warn was not called) to ensure the same-token branch aggregates case-insensitively across multiple trades.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/explorer/src/api/operator/types.ts`:
- Around line 104-108: Change the protocolFeeTokenAddress property in the
OperatorTrade type from string to AddressKey (replace protocolFeeTokenAddress?:
string with protocolFeeTokenAddress?: AddressKey) and update all callers; then
replace any address comparisons that use toLowerCase() in the sameDenomination
logic with the address helpers — in apps/explorer/src/utils/operator.ts and
apps/explorer/src/components/orders/GasFeeDisplay/index.tsx use
areAddressesEqual(a,b) or normalize using getAddressKey(addr) before comparing
instead of addr.toLowerCase(), ensuring you import AddressKey, areAddressesEqual
and getAddressKey from the existing address utility module.
In `@apps/explorer/src/components/orders/GasFeeDisplay/index.tsx`:
- Around line 67-69: Replace direct .toLowerCase() comparisons with the cow-sdk
address helper: update the sameDenomination computation in GasFeeDisplay to call
areAddressesEqual(executedFeeToken, protocolFeeTokenAddress) instead of
comparing lowered strings; change formatFee and resolveToken signatures to
accept an AddressKey parameter type and inside each use
areAddressesEqual(tokenAddress, addressKey) (or similar) to determine equality
rather than calling toLowerCase() on addresses, and update any internal logic
that relied on string lowercasing to use the areAddressesEqual helper for all
address comparisons.
In `@apps/explorer/src/utils/operator.ts`:
- Around line 461-468: The fees grouping currently keys feesByToken by raw token
strings causing checksum-casing splits; change feesByToken to Map<AddressKey,
BigNumber> and when iterating executedProtocolFees use getAddressKey(token) as
the key (while still validating token exists) so amounts aggregate under a
normalized AddressKey (refer to feesByToken and the trades.forEach block). Also
replace the string lower-case comparison in sameDenomination with a call to
areAddressesEqual(tokenA, tokenB) per the address-comparison guideline to ensure
correct equality checks across checksum variants.
---
Nitpick comments:
In `@apps/explorer/src/test/utils/operator/orderFees.test.ts`:
- Around line 81-89: Add a regression test in orderFees.test.ts that verifies
aggregation across mixed-checksum token strings: create two trades using
makeTrade — one with token FEE_TOKEN and one with FEE_TOKEN.toLowerCase() — pass
them to getFees alongside an order from makeOrder with executedFeeToken set to a
different-cased FEE_TOKEN and totalFee large enough to split protocol/network
fees, then assert the returned protocolFees equals the sum of both trade amounts
(as a single token aggregation) and networkCosts equals the expected remainder;
also assert no warning was emitted (e.g., result.warnings is empty or
console.warn was not called) to ensure the same-token branch aggregates
case-insensitively across multiple trades.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 834ec6c8-a987-4d6c-a53b-f60aacd95af2
📒 Files selected for processing (9)
apps/explorer/src/api/operator/types.tsapps/explorer/src/components/AppDataRowContent/AppDataRowContent.tsxapps/explorer/src/components/common/ShowMoreButton.tsxapps/explorer/src/components/orders/GasFeeDisplay/GasFeeDisplay.stories.tsxapps/explorer/src/components/orders/GasFeeDisplay/index.tsxapps/explorer/src/components/orders/NumbersBreakdown/index.tsxapps/explorer/src/components/orders/OrderDetails/index.tsxapps/explorer/src/test/utils/operator/orderFees.test.tsapps/explorer/src/utils/operator.ts
Collaborator
|
@jmg-duarte can you take a look at the failing unittests? https://github.com/cowprotocol/cowswap/actions/runs/26631936508/job/78482889272?pr=7588 |
Collaborator
|
Additionally, something is weird. Checking a random order, I see this: 
The network costs token is not being decoded for some reason. In this case, the token is WETH. However, it's also an edge case as it's an ethflow order, so the sell token should actually be displayed as ETH. |
|
I was expecting a change towards only displaying protocol fees. We will not be able to reconstruct network fees. (I am assuming that |
fairlighteth
self-requested a review
June 2, 2026 08:00
fairlighteth
reviewed
Jun 2, 2026
fairlighteth
left a comment
fairlighteth
left a comment
Contributor
There was a problem hiding this comment.
⚠️ AI Review (Codex GPT-5, worked 4m): fee breakdown uses only the current fills page
Finding: Aggregate fee breakdown from all order trades, not the paginated fills table page
- Location:
apps/explorer/src/components/orders/OrderDetails/index.tsx:160 enrichOrderFromTradescallsgetFees(order, trades), buttradescomes fromuseOrderTrades(order, tableState.pageOffset, tableState.pageSize), which fetcheslimit + 1records and then slices back to the current page.- For a partially fillable order with more than 10 fills, the overview breakdown only sums the visible page. Page 1 omits later fills; after paging in the Fills tab, returning to Overview can show different fee/network-cost values for the same order.
- Impact: order-level costs become undercounted and page-dependent.
Suggested fix
- Use a complete trade set or dedicated aggregate for the order-level fee breakdown, or only render the breakdown when all trades needed for the aggregate have been loaded.
- Add coverage for an order with more fills than
RESULTS_PER_PAGEsoprotocolFeesincludes all fills and does not change withpageOffset.
Review scope and related context
This is separate from the existing review comments, which already cover:
- Address typing/comparison: resolved with
AddressKey,getAddressKey, andareAddressesEqual. - EthFlow WETH/ETH fee-token formatting: already reported by @alfetopito.
- Whether network fees should be displayed at all: already raised by @fhenneke.
- Mixed-checksum aggregation coverage: already suggested by CodeRabbit.
🤖 Prompt for AI agents
Verify this finding against current code. Fix only if still valid, keep the change minimal, and validate with targeted tests.
Context:
- apps/explorer/src/components/orders/OrderDetails/index.tsx:160 calls getFees(order, trades).
- apps/explorer/src/explorer/components/OrderWidget/index.tsx:36 passes useOrderTrades(order, tableState.pageOffset, tableState.pageSize).
- apps/explorer/src/hooks/useOperatorTrades.ts:69 fetches limit + 1 and lines 119-122 slice back to the current page.
- Fee breakdown should be order-level, not dependent on the currently selected fills page.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/explorer/src/components/orders/GasFeeDisplay/index.tsx (1)
105-123: 💤 Low valueMinor: Redundant
getAddressKeycall on already-normalized address.Line 122 calls
getAddressKey(address)butaddressis already anAddressKey(fromfee.tokenAddress, which was normalized viagetAddressKey(token)inoperator.tsline 482). SincefeeTokensByKeyis also keyed bygetAddressKey(...)results, you can usefeeTokensByKey.get(address)directly.♻️ Simplify lookup
- return feeTokensByKey.get(getAddressKey(address)) || undefined + return feeTokensByKey.get(address) || undefined🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/explorer/src/components/orders/GasFeeDisplay/index.tsx` around lines 105 - 123, In resolveToken, avoid the redundant normalization by using the already-normalized AddressKey directly when looking up feeTokensByKey: replace the final lookup that uses feeTokensByKey.get(getAddressKey(address)) with feeTokensByKey.get(address). This touches the resolveToken function and the feeTokensByKey map usage; no other behavior changes needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/explorer/src/components/orders/GasFeeDisplay/index.tsx`:
- Around line 105-123: In resolveToken, avoid the redundant normalization by
using the already-normalized AddressKey directly when looking up feeTokensByKey:
replace the final lookup that uses feeTokensByKey.get(getAddressKey(address))
with feeTokensByKey.get(address). This touches the resolveToken function and the
feeTokensByKey map usage; no other behavior changes needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0cdd163f-1d15-40e1-b367-37dab2ea2e25
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
apps/cowswap-frontend/package.jsonapps/explorer/package.jsonapps/explorer/src/api/operator/types.tsapps/explorer/src/components/orders/DetailsTable/detailsTableTooltips.tsxapps/explorer/src/components/orders/GasFeeDisplay/GasFeeDisplay.stories.tsxapps/explorer/src/components/orders/GasFeeDisplay/index.tsxapps/explorer/src/components/orders/OrderDetails/index.tsxapps/explorer/src/test/data/operator.tsapps/explorer/src/test/utils/operator/orderFees.test.tsapps/explorer/src/utils/operator.tsapps/sdk-tools/package.jsonapps/widget-configurator/package.jsonlibs/analytics/package.jsonlibs/balances-and-allowances/package.jsonlibs/common-const/package.jsonlibs/common-hooks/package.jsonlibs/common-utils/package.jsonlibs/core/package.jsonlibs/currency/package.jsonlibs/ens/package.jsonlibs/events/package.jsonlibs/multicall/package.jsonlibs/permit-utils/package.jsonlibs/tokens/package.jsonlibs/types/package.jsonlibs/ui/package.jsonlibs/wallet/package.jsonlibs/widget-lib/package.json
✅ Files skipped from review due to trivial changes (18)
- libs/analytics/package.json
- libs/ui/package.json
- libs/widget-lib/package.json
- libs/wallet/package.json
- libs/balances-and-allowances/package.json
- libs/permit-utils/package.json
- libs/common-utils/package.json
- libs/multicall/package.json
- libs/currency/package.json
- libs/common-const/package.json
- libs/core/package.json
- libs/common-hooks/package.json
- apps/widget-configurator/package.json
- libs/events/package.json
- libs/types/package.json
- apps/sdk-tools/package.json
- libs/ens/package.json
- apps/explorer/package.json
…r details Derive the breakdown client-side from `trades[].executedProtocolFees`: sum protocol fees per token, then split `order.totalFee` into network costs vs. protocol fees — subtracting only when both are denominated in the same token, since the protocol fee comes from the surplus token while `totalFee` lives in `executedFeeToken`.
… fills page The order overview's protocol fee breakdown was computed via getProtocolFees(trades) where `trades` is the currently selected Fills table page. Since the trades API paginates server-side, orders with more fills than one page undercounted the breakdown and showed different values as the user paged. Add a dedicated useOrderProtocolFees hook that pages through every trade of the order and derives the order-level breakdown independently of the Fills table page. enrichOrderFromTrades now consumes this precomputed set instead of recomputing from the visible page. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Surface each volume fee's policy factor as basis points (e.g. "Volume fee (2 bps)") so an order's protocol vs partner volume fees—identically labeled by the API, which doesn't expose fee owner—can be told apart. The factor is already in executedProtocolFees, just unused until now. Also skip non-positive fees so a policy that charges nothing doesn't add a "0" row, and stack the breakdown table under the show-more toggle (via DetailRow stack) instead of beside it, trimming the gap above it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adapt the order details "Costs and fees" breakdown to the orderbook's new
`gasCost` field:
- Revert the row label from "Protocol fees" back to "Costs and fees" and
update its tooltip (network costs are now included).
- Thread `order.gasCost` (native-token wei, summed across fills) through the
explorer types and `transformOrder`. The SDK type doesn't carry it yet, so
`RawOrder` extends it locally; it's parsed defensively.
- Gate the UI on gasCost: orders without a recorded gas cost (or 0) keep the
legacy combined-fee display; orders with one get the new breakdown.
- New breakdown: a "Network costs" line (gasCost in the native token), then the
protocol fee, then numbered partner fees.
- Derive the fee breakdown by applied position instead of token+type+factor.
Fee policies are fixed per order, so position N is the same policy in every
fill; position 0 is the protocol's own fee (volume relabeled "Protocol fee",
surplus / price-improvement keep their labels), and the fees applied after it
are partner fees, numbered by order ("Partner N volume fee" / "Partner N
price improvement share").
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Network costs are paid in the native token (ETH) and protocol fees are often taken in the wrapped native (WETH) — the same asset. Fold wrapped-native into the native bucket for the headline total so it reads as one figure (e.g. "0.00024 ETH") instead of "x ETH, y WETH". The per-row breakdown still shows each fee in the token it was charged in. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
azebuado
reviewed
Jul 24, 2026
azebuado
left a comment
azebuado
left a comment
Contributor
There was a problem hiding this comment.
Looking good!
Superficial tests, did not inspect the code yet:
| trade | order | pic | comments |
|---|---|---|---|
| weth -> usdc | order |  |
native + surplus token mixed |
| usdc -> weth | order |  |
all in native summed up, nice! |
| usdc -> cow | order |  |
native + surplus token mixed |
| eth -> gno | order |  |
ethflow, native + surplus mixed |
General comments:
- The fact that it's mixed is not a must for now, we discussed this before
- It does not yet have the mappings of the names (protocol fee, price improvement whatever), and that will also come at a later stage, right?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace the single combined "fee" figure on the order details page with a costs & fees breakdown:
gasCost(read offRawOrderuntil the SDK ships it).executedProtocolFees, per token + fee type — position0is the protocol fee, later ones are numbered partner fees.gasCostfall back to the old combined-fee display.Based on #5024
To Test
gasCostSummary by CodeRabbit
New Features
Improvements
Tests
Chores